Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add authenticated session to app server context #3157

Merged
merged 67 commits into from
Feb 12, 2024

Conversation

apedroferreira
Copy link
Member

@apedroferreira apedroferreira commented Feb 1, 2024

Add authenticated session to server context so that it can be retrieved in custom functions.
Had to move some things around between @mui/toolpad-core and @mui/toolpad-utils, let me know if it could be set up differently.
Also updated docs with an example, and covered in tests.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 8, 2024
@apedroferreira
Copy link
Member Author

apedroferreira commented Feb 8, 2024

Looks like with the recent changes we can redirect to the sign-in page with React Router: 172d2c3

About the flaky Firefox test mentioned in #3179 (comment) I just added those console errors to the ignore list in this test, i think it should be fine to do that?
From what I found it looks like in the checks that see if the user gets redirected to the sign-in page, that redirect interrupts some network requests and in Firefox that leads to showing some console errors. This discussion seems to be related to that https://bugzilla.mozilla.org/show_bug.cgi?id=1280189#c7, and seems like they're just browser differences...

@github-actions github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Feb 9, 2024
| :---------- | :----------------------------------------------------------------- | :---------------------------------------------------------------------------------- |
| `cookies` | `Record<string, string>` | A dictionary mapping cookie name to cookie value. |
| `setCookie` | `(name: string, value: string) => void` | Use to set a cookie `name` with `value`. |
| `user` | `{ name: string; email: string; avatar: string; roles: string[] }` | Get current [authenticated](/toolpad/concepts/authentication/) logged-in user data. |
Copy link
Member

@Janpot Janpot Feb 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Let's add a separate table for this interface?
  • Would it make sense to call this session instead of user?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

session sounds good, it's what it's called in @auth/core too, user might be a bit too generic.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made these changes, ended up having session in context which has a user property with this data.
Also covered this in one of the authentication tests (since the user needs to be logged-in to see the context session data).

@apedroferreira apedroferreira changed the title Add logged-in user to app server context Add authentication session to app server context Feb 9, 2024
@apedroferreira apedroferreira changed the title Add authentication session to app server context Add authenticated session to app server context Feb 9, 2024
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 12, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 12, 2024
@apedroferreira apedroferreira enabled auto-merge (squash) February 12, 2024 19:32
@apedroferreira apedroferreira merged commit 9406c7d into mui:master Feb 12, 2024
11 checks passed
@Janpot
Copy link
Member

Janpot commented Feb 13, 2024

@apedroferreira Would it make sense to also add user info in the page state for components to bind to? e.g. like https://docs.retool.com/reference/apps/global/objects/current_user

@apedroferreira
Copy link
Member Author

@apedroferreira Would it make sense to also add user info in the page state for components to bind to? e.g. like https://docs.retool.com/reference/apps/global/objects/current_user

It would be nice, I've created an issue #3209.
Marked it as a good first issue but not 100% sure if I'm underestimating how difficult it would be.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation feature: Queries Making new API requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants